Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race conditions in signal handling #38

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

HED-jzignego
Copy link
Contributor

@HED-jzignego HED-jzignego commented Feb 15, 2022

Commits:

  • src/socketpair.cpp: remove dataCheck timer
  • src/mainwindow.cpp: don't keep delayedResize timer
  • src/mainwindow.cpp: don't draw on screen until ready
  • src/unixsignals.cpp: fix signal handler
  • src/socketpair.*: log all connection errors
  • socketpair.cpp: use Qt::QueuedConnection for newConnection
  • Don't add signal handlers until socket connection
  • src/anyoption.cpp: Add missing { } after if
  • src/socketpair.cpp: Fix inconsistent indentation
  • convert all files to unix line endings

@HED-jzignego HED-jzignego force-pushed the line-endings branch 2 times, most recently from 8f84502 to 5c8547a Compare February 28, 2022 21:43
If we add the OS signal handlers (via `handler->start()`) before the
client connection to the `QTcpServer` has been completed, we will
deadlock by writing to a socket that doesn't exist yet.

So instead, we emit a Qt Signal once the client has been connected to
the server in socketpair.cpp. unixsignals.cpp then connects that signal
to its `start()` function as a slot. Then no matter what, the signal
handler will not be installed until we are able to write to the socket.
If we receive an OS signal before that, the default OS signal handler
for the signal will be used instead. For example, if we receive SIGTERM
before that, then the process will exit.
We can't handle signals (like SIGTERM) until after the event loop has
started since we call `QApplication::exit(0);` when we receive SIGTERM
or SIGINT, and that function won't do anything if we haven't started the
event loop yet. See https://doc.qt.io/qt-5/qcoreapplication.html#exit

A Qt signal and slot, connected by Qt::QueuedConnection however, means
that the signal won't be delivered to the slot until the next iteration
of the event loop. Which means that if the event loop hasn't started
yet, it won't be delivered until the event loop starts.
@HED-jzignego HED-jzignego force-pushed the line-endings branch 2 times, most recently from d067076 to 4e03db9 Compare March 2, 2022 20:51
QTcpSocket and QTcpServer emit an Qt signal on error, so we create slots
to receive those signals, and print the error that ocurred to stderr
through qCritical. qCritical gives the admin flexibility in choosing
whether or not to make those errors fatal or not through setting or not
the environment variable `QT_FATAL_CRITICALS`. See
https://doc.qt.io/qt-5/qtglobal.html#qCritical
Fix two issues in the signal handler function:
* Use `volatile std::sig_atomic_t` for the variable that will be written
to the socket. See
https://en.cppreference.com/w/cpp/utility/program/sig_atomic_t and
https://en.cppreference.com/w/cpp/utility/program/signal

* Don't try to print anything to the log or the console in the signal
handler. We were calling `qDebug()` which seems convenient for debugging
the signal handler, however, `qDebug()` calls `gettimeofday()`, which is
not signal safe. The list of signal safe functions is here:
https://pubs.opengroup.org/onlinepubs/000095399/functions/xsh_chap02_04.html#tag_02_04_01
which notably does _not_ include `gettimeofday()`. Unfortunately there
is virtually no way to log something safely from inside a signal handler
itself. We can log stuff once we've passed it along via Qt Signals and
Slots, but not inside the OS signal handler itself. Also see
https://stackoverflow.com/questions/53155166/is-gettimeofday-async-signal-safe-and-can-it-cause-deadlock-if-used-in-signal
and https://man7.org/linux/man-pages/man7/signal-safety.7.html
The OS signal handler can't be installed until after the Qt event loop
starts since it eventually calls `QApplication::exit(0);`, and that will
cleanup anything we draw on the screen. Thus we don't want to connect
any slots that touch the what's in focus or draw on the screen until the
OS signal handler is ready. `mainwindow->init()` is called in
`src/main.cpp` before `app.exec()`, thus we don't want to connect any
slots that draw on screen in `mainwindow->init()`. Instead,
`mainwindow->init()` will connect the `setupWindow()` slot, to the the
`signalHandlerInstalled` Qt signal emitted by `this->handler`. At that
point the OS signal handler has been installed and it is safe to draw on
the screen.
We don't need to keep the delayedResize timer around for later. We will
potentially use the delayedLoad timer again, but not the delayedResize
timer after it has been used the first time.
@HED-jzignego HED-jzignego changed the title Line endings Fix race conditions in signal handling Mar 7, 2022
Now that the race conditions in starting the signal handling are
resolved, we can get rid of the dataCheck timer, and just use the
readyRead signal from the serverConnection socket.
@HED-jzignego
Copy link
Contributor Author

HED-jzignego commented Mar 11, 2022

@sergey-dryabzhinsky I decided to abandon the SocketPair approach since that wouldn't have worked with Windows. I took this approach instead, this code is working very well both on our devices and on my desktop. Wanna merge it?

I have not tested it on Windows however, but I tried to make it as Windows friendly as possible.

@HED-jzignego
Copy link
Contributor Author

@sergey-dryabzhinsky hey, do you want to merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants